Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds checks for container and bootc hosts #2110

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Adds checks for container and bootc hosts #2110

merged 1 commit into from
Jul 22, 2024

Conversation

jmarrero
Copy link
Contributor

@jmarrero jmarrero commented Jul 16, 2024 •
edited
Loading

This changes the is_container() func for is_bootc_host() and updates the logic. This should detect on all ostree and bootc hosts to date that are not using bootc usroverlay or ostree admin unlock for development purposes.

@pep8speaks
Copy link

pep8speaks commented Jul 16, 2024 •
edited
Loading

Hello @jmarrero! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-07-22 13:47:34 UTC

cgwalters
cgwalters reviewed Jul 16, 2024
View reviewed changes
dnf/util.py Outdated Show resolved Hide resolved
dnf/util.py Outdated Show resolved Hide resolved
dnf/util.py Outdated Show resolved Hide resolved
dnf/util.py Outdated Show resolved Hide resolved
dnf/util.py Outdated Show resolved Hide resolved
@jmarrero
Copy link
Contributor Author

did a quick test on a bootc host and get the message by default, and can use dnf to install software in a transient way when I do bootc usroverlay or run it in a container.

@dcantrell dcantrell self-assigned this Jul 17, 2024
@jmarrero jmarrero marked this pull request as ready for review July 17, 2024 14:04
@jmarrero
Copy link
Contributor Author

This is missing the links to the docs described in: https://issues.redhat.com/browse/SWM-1313

jlebon
jlebon approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple. Looks sane to me overall.

dnf/cli/cli.py Show resolved Hide resolved
dnf/util.py Show resolved Hide resolved
dnf/util.py Show resolved Hide resolved
@dcantrell dcantrell self-requested a review July 17, 2024 17:07
@travier travier mentioned this pull request Jul 17, 2024
Closed
@travier
Copy link

travier commented Jul 17, 2024

See: #2108

@dcantrell
Copy link
Contributor

@jmarrero can you squash these commits in to a single for the purposes of the PR? In the commit message please explain the fix and reference that it resolves RHEL-49670 and RHEL-49671

@jlebon
Copy link
Contributor

jlebon commented Jul 18, 2024

For the docs URL, we could pull it from /etc/os-release. Possibly DOCUMENTATION_URL, which is an existing field, but we could also add a separate field too. This makes it so that different distros can reference different pages. (And also so that we don't have to block on getting docs ready.)

@jmarrero
Copy link
Contributor Author

@jmarrero can you squash these commits in to a single for the purposes of the PR? In the commit message please explain the fix and reference that it resolves RHEL-49670 and RHEL-49671

done.

ppisar
ppisar reviewed Jul 19, 2024
View reviewed changes
dnf/util.py Outdated Show resolved Hide resolved
ppisar
ppisar reviewed Jul 19, 2024
View reviewed changes
dnf/cli/cli.py Outdated Show resolved Hide resolved
@ravanelli
Copy link

ravanelli commented Jul 19, 2024 •
edited
Loading

For the docs URL, we could pull it from /etc/os-release. Possibly DOCUMENTATION_URL, which is an existing field, but we could also add a separate field too. This makes it so that different distros can reference different pages. (And also so that we don't have to block on getting docs ready.)

That’s a thing we would need to align with the RHEL/FEDORA team to have it there? I mean, that’s not something it up to us to decided if we can add it there or not. We may have a no as an answer.
Should we probably try to look at this option asap?

@jmarrero
Copy link
Contributor Author

Talked to Colin about it and we can add it as part of the build process of our image-mode systems. We should probably split this into it's own issue.
I have a draft here #2112 to rebase once we have merged this PR. We can "bike shed" the message and process to get it there.

@cgwalters
Copy link
Contributor

I don't think this can be the first case where we've wanted to reference docs from software we ship. It is a complex topic. Don't get me wrong, I'm not totally fixated on the idea the docs have to be online - it could also be a man dnf-bootc or something (but, while that makes sense it hits on the problem we don't ship man pages in many targets where want this today).

Many systemd units for example that we ship reference Documentation= pointing to an upstream URL. That upstream URL could be somewhere dnf-upstream owned too.

@jmarrero jmarrero mentioned this pull request Jul 19, 2024
Merged
@ppisar
Copy link
Contributor

ppisar commented Jul 22, 2024

Could you also remove "import json" and "import subprocess" lines from dnf/util.py? Your patch removed the only reason for their imports.

@ppisar
Copy link
Contributor

ppisar commented Jul 22, 2024

Thanks for renaming the function. Please change the name also in the commit message.

@jmarrero
Update ostree/bootc host system check.
575f69b 
This changes the is_container() func for _is_bootc_host()
and updates the logic and message. This should detect on
all ostree and bootc hosts to date that are not using
bootc usroverlay or ostree admin unlock for development
purposes.

resolves: #RHEL-49670, RHEL-49671
@jmarrero
Copy link
Contributor Author

Could you also remove "import json" and "import subprocess" lines from dnf/util.py? Your patch removed the only reason for their imports.

done

@jmarrero
Copy link
Contributor Author

Thanks for renaming the function. Please change the name also in the commit message.

done

@dcantrell dcantrell merged commit 6120fe5 into rpm-software-management:master Jul 22, 2024
8 of 9 checks passed
@travier travier mentioned this pull request Aug 20, 2024
Merged
@ppisar ppisar mentioned this pull request Sep 5, 2024
Merged
@jlebon jlebon mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcurlej mcurlej mcurlej left review comments

@cgwalters cgwalters cgwalters left review comments

@dcantrell dcantrell dcantrell approved these changes

@jlebon jlebon jlebon approved these changes

@ppisar ppisar ppisar approved these changes

Assignees

@dcantrell dcantrell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jmarrero @pep8speaks @travier @dcantrell @jlebon @ravanelli @cgwalters @ppisar @mcurlej